Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add From<[Elem; N]> for Packed Array and Optimize From<Vec<Elem>> #827

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

Dheatly23
Copy link
Contributor

When making From array impl, i realized it and Vec are very similiar in semantics.

From array is useful to create small PackedArray quickly.

No From<Box<[Elem]>> since boxed slice can be converted into Vec as intermediate.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-827

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Aug 1, 2024
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

Not yet very sure about the safety invariant, added some comments. Do we really gain performance with it?

godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
}
ret.resize(len);
let ptr = vec.as_ptr();
// SAFETY: Data is moved and the original vector is forcibly set to empty.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The // SAFETY assertion is not just a regular comment that explains what happens, but it needs to reason why and how the safety assumptions (specified in the function docs) are upheld.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also be good to have an empty line before it.

godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
@Dheatly23 Dheatly23 force-pushed the packed-array-from-array branch 2 times, most recently from ac68f66 to eeb06e2 Compare August 1, 2024 10:28
Comment on lines 475 to 474
let (len, mut ret) = match vec.len() {
0 => return Self::new(),
len => (len, Self::default_with_size(len)),
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessarily hard to understand, could you just write a

if vec.is_empty() {
    return Self::new();
}

instead?

Copy link
Contributor Author

@Dheatly23 Dheatly23 Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something like this?

if vec.is_empty() {
    return Self::new();
}
let len = vec.len();
let mut ret = Self::default_with_size(len);

I don't know, but it doesn't save enough space to justify default_with_size and stand out with above Froms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe I was too optimistic with the code reuse here... Otherwise it's OK to not have default_with_size. More important than a bit of duplication or fewer lines of code is readability (which is problematic with the match version).

godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
@Bromeon
Copy link
Member

Bromeon commented Aug 1, 2024

It's a bit annoying that Godot doesn't have a "resize without default-construct" (à la Vec::set_len), nor a "reserve" function.

But I don't see a way around default-constructing each element and then dropping it. "Dropping" is probably simplest done via = moves (which calls drop on lhs operand). Even if you want to use copy_nonoverlapping, you need to individually call drop_in_place() on each default-constructed element beforehand.

The alternative is appending elements to an empty collection (e.g. involving Vec::drain), which may be more expensive due to reallocations. Would need measurements to be sure.

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments, thanks for the update!

Is there a concrete use case for supporting array conversions? People are always quick to add From with arrays, but in reality very few people use actual arrays, it's almost always Vec due to the dynamic nature. The syntax you mentioned is already possible with slices (although it can be tiny bit more expensive due to copy):

PackedInt32Array::from(&[1, 2])

Not against it, just wondering 🙂

godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
@Dheatly23 Dheatly23 force-pushed the packed-array-from-array branch 2 times, most recently from b9c97a4 to 3d92626 Compare August 2, 2024 07:04
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an integration test for From<Vec>? Maybe with a packed array type that isn't yet used, e.g. Color...

godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/collections/packed_array.rs Outdated Show resolved Hide resolved
array
}

/// Moves elements from slice.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Moves elements from slice.
/// Moves elements from slice.

Comments should express more than the function name, or be omitted.

Here, you could mention something like

Suggested change
/// Moves elements from slice.
/// Drops all elements in self and replaces them with ones in provided pointer range.

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the continuous improvements!

@Bromeon Bromeon added this pull request to the merge queue Aug 2, 2024
Merged via the queue into godot-rust:master with commit 62f9caf Aug 2, 2024
14 checks passed
@Dheatly23 Dheatly23 deleted the packed-array-from-array branch August 2, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants